Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #604 - remove line overwriting for scenario outlines in cucumber formatter #605

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Fixes #604 - remove line overwriting for scenario outlines in cucumber formatter #605

merged 1 commit into from
Jan 31, 2024

Conversation

glibas
Copy link
Member

@glibas glibas commented Jan 26, 2024 •
edited
Loading

🤔 What's changed?

Removing line overwrite from cucumber formatter

if len(pickle.AstNodeIds) == 2 {
        _, row := feature.FindExample(pickle.AstNodeIds[1])
        line = row.Location.Line
    }

⚡️ What's your motivation?

The above code from cucumber/godog/fmt_cucumber.go overwrites the line property values of steps in cucumber reports (for Scenario Outlines only, regular Scenarios reports look good).

As a result, the step line property corresponds to the line in the Examples section rather than to the actual location of step within the feature file.

Due to the above, external tools that process the reports and use the line property for sorting the steps, display the steps out of order.

Other officially supported cucumber implementations (java, js, python, etc.) are generating reports correctly for Scenario Outline - line property corresponds to the actual location of step within the file rather than the line in Examples section.

🏷️ What kind of change is this?

  • 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@github-actions GitHub Actions
Copy link

Go API Changes

# summary
Inferred base version: v0.13.0
Suggested version: v0.13.1

@glibas glibas force-pushed the feature/cucumber-formatter branch 3 times, most recently from 5a90497 to 8830791 Compare January 26, 2024 15:26
@codecov Codecov
Copy link

codecov bot commented Jan 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5a5631a) 83.22% compared to head (4632c0a) 83.21%.

❗ Current head 4632c0a differs from pull request most recent head 47145eb. Consider uploading reports for the commit 47145eb to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #605      +/-   ##
==========================================
- Coverage   83.22%   83.21%   -0.02%     
==========================================
  Files          28       28              
  Lines        3416     3413       -3     
==========================================
- Hits         2843     2840       -3     
  Misses        458      458              
  Partials      115      115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glibas glibas requested a review from vearutop January 26, 2024 15:28
@glibas
Copy link
Member Author

glibas commented Jan 26, 2024 •
edited
Loading

Hi @roskee @vearutop could you please take a look

@glibas glibas requested a review from roskee January 26, 2024 19:51
@roskee
Copy link
Member

roskee commented Jan 26, 2024

@glibas could you perhaps share an example to reproduce the problem so that we can test it as well. And also link the original issue #604 so that it will be closed once this pull request is done. Thanks 🙂

@glibas
Copy link
Member Author

glibas commented Jan 26, 2024

Hi @roskee ,
The issue is well illustrated in this test cucumber.json output scenario_outline. The line of the step is being overwritten with the line of the example in Examples table, so all the steps within one scenario outline instance have the same line number causing
report generators which are using cucumber.json output to display steps in random order, since they rely on the line number.

@glibas glibas changed the title Remove line overwriting for scenario outlines in cucumber formatter Fixes Remove line overwriting for scenario outlines in cucumber formatter Jan 26, 2024
@glibas glibas changed the title Fixes Remove line overwriting for scenario outlines in cucumber formatter Fixes #604 Remove line overwriting for scenario outlines in cucumber formatter Jan 26, 2024
@glibas glibas changed the title Fixes #604 Remove line overwriting for scenario outlines in cucumber formatter Fixes #604 - remove line overwriting for scenario outlines in cucumber formatter Jan 26, 2024
@glibas
Copy link
Member Author

glibas commented Jan 27, 2024

Hi @roskee, also as an example can be used this scenario Scenario: Support of Feature Plus Scenario Outline With Steps which can be executed before the change and after the change to observe changes in lines numbering

@glibas
Copy link
Member Author

glibas commented Jan 30, 2024

Hi @roskee apologies for chasing on this, would you be able to have a look?

@roskee
Copy link
Member

roskee commented Jan 30, 2024

Sorry for being late. I was very busy. I will take a look and let you know my thoughts.

@glibas
Copy link
Member Author

glibas commented Jan 30, 2024 •
edited
Loading

No problem, thank you. In essence the change aligns godog cucumber json reporting behavior for scenario outlines with other cucumber implementations so hopefully should be straight forward.

@glibas glibas removed the request for review from vearutop January 30, 2024 21:15
roskee
roskee approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@roskee roskee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right way. I am not sure why step lines were being overwritten in the first place 🤷‍♂️

@glibas
Copy link
Member Author

glibas commented Jan 31, 2024

Thank you @roskee for confirming!

@glibas glibas merged commit 250d69a into main Jan 31, 2024
6 checks passed
@glibas glibas deleted the feature/cucumber-formatter branch January 31, 2024 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roskee roskee roskee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@glibas @roskee